Fix: Close editor and import dialog after successful API Definition i… #1221
Conversation
…mport - Added mechanism to close ImportDefinition dialog from parent component - Modified ImportDefinition.jsx to expose close function via onImportDialogClose prop - Updated APIDefinition.jsx to store and call the close dialog function - After successful import save, both editor and import dialog now close automatically - Reset isImporting state to prevent subsequent issues Fixes #52
ranuka-laksika
requested review from
Krishanx92,
lasanthaS,
piyumaldk and
tharikaGitHub
as code owners
WalkthroughModified APIDefinition and ImportDefinition components to establish bidirectional communication for managing the import dialog lifecycle. APIDefinition now passes a callback setter to ImportDefinition, which registers a cancel function. Upon successful import/update, the stored callback is invoked to automatically close the import dialog. Changes
Sequence Diagram(s)sequenceDiagram
participant Parent as APIDefinition
participant Child as ImportDefinition
participant User as User
User->>Parent: Click "Import Definition"
Parent->>Child: Render with onImportDialogClose callback
Child->>Child: useEffect: Register cancel function
Child->>Parent: onImportDialogClose(handleAPIDefinitionImportCancel)
Parent->>Parent: Store callback in closeImportDialog state
User->>Child: Select "Edit and Import"
User->>User: Make changes in editor
User->>Child: Click "Import Content"
Child->>Parent: Submit definition
Parent->>Parent: Process import successfully
Parent->>Parent: Invoke closeImportDialog callback
Parent->>Child: Trigger handleAPIDefinitionImportCancel
Child->>Child: Close import dialog
Parent->>Parent: Close editor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/APIDefinition/ImportDefinition.jsx (1)
120-127: Consider adding exhaustive dependencies to useEffect.The
useEffecthook referenceshandleAPIDefinitionImportCancelin its body but doesn't include it in the dependency array. While this works in practice (sinceisAsyncAPIis stable for a given API), it may trigger linting warnings.Consider either:
- Adding
handleAPIDefinitionImportCancelto the dependency array, or- Wrapping
handleAPIDefinitionImportCancelwithuseCallbackto stabilize its referenceExample with useCallback:
+const handleAPIDefinitionImportCancel = useCallback(() => { + isAsyncAPI ? setAsyncAPIDefinitionImport(false) : setOpenAPIDefinitionImport(false); + inputsDispatcher({ action: 'inputValue', value: null }); +}, [isAsyncAPI]); -const handleAPIDefinitionImportCancel = () => { - // eslint-disable-next-line no-unused-expressions - isAsyncAPI ? setAsyncAPIDefinitionImport(false) : setOpenAPIDefinitionImport(false); - inputsDispatcher({ action: 'inputValue', value: null }); -}; // Expose close dialog function to parent component useEffect(() => { if (onImportDialogClose) { - onImportDialogClose(() => { - handleAPIDefinitionImportCancel(); - }); + onImportDialogClose(handleAPIDefinitionImportCancel); } -}, [onImportDialogClose]); +}, [onImportDialogClose, handleAPIDefinitionImportCancel]);portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/APIDefinition/APIDefinition.jsx (1)
610-617: Add defensive null check before invoking callback.The code checks if
this.state.closeImportDialogexists at line 614, which is good. However, it's accessing state directly at line 615. While this works, consider destructuring state for consistency and readability.Apply this diff for improved consistency:
+const { closeImportDialog } = this.state; + // Close editor and import dialog after successful import if (this.state.isImporting) { this.setState({ openEditor: false, isImporting: false }); - if (this.state.closeImportDialog) { - this.state.closeImportDialog(); + if (closeImportDialog) { + closeImportDialog(); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/APIDefinition/APIDefinition.jsx(5 hunks)portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/APIDefinition/ImportDefinition.jsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/APIDefinition/APIDefinition.jsx (1)
portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/APIDefinition/ImportDefinition.jsx (1)
ImportDefinition(65-501)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-product
🔇 Additional comments (5)
portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/APIDefinition/ImportDefinition.jsx (1)
18-18: LGTM: useEffect import added.The import is necessary for the new lifecycle hook that registers the close callback with the parent component.
portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/APIDefinition/APIDefinition.jsx (4)
174-174: LGTM: State field for storing the close callback.The
closeImportDialogstate field is properly initialized to store the child component's close function.
191-191: LGTM: Method binding in constructor.The
setImportDialogClosemethod is correctly bound to maintain properthiscontext.
526-532: LGTM: Callback registration method.The implementation correctly receives and stores the close function from the ImportDefinition child component.
871-873: LGTM: Callback prop correctly passed to child.The
onImportDialogCloseprop is properly passed to the ImportDefinition component, establishing the parent-child communication channel for dialog lifecycle management.
| */ | ||
| export default function ImportDefinition(props) { | ||
| const { setSchemaDefinition, editAndImport } = props; | ||
| const { setSchemaDefinition, editAndImport, onImportDialogClose } = props; |
There was a problem hiding this comment.
Add PropTypes validation for the new prop.
The onImportDialogClose prop is missing from the PropTypes definition at lines 503-505. Add validation to document the expected function signature.
Add this to the PropTypes definition:
ImportDefinition.propTypes = {
setSchemaDefinition: PropTypes.func.isRequired,
+ editAndImport: PropTypes.func,
+ onImportDialogClose: PropTypes.func,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { setSchemaDefinition, editAndImport, onImportDialogClose } = props; | |
| ImportDefinition.propTypes = { | |
| setSchemaDefinition: PropTypes.func.isRequired, | |
| editAndImport: PropTypes.func, | |
| onImportDialogClose: PropTypes.func, | |
| }; |
🤖 Prompt for AI Agents
In
portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/APIDefinition/ImportDefinition.jsx
around line 66 and update the PropTypes block at lines 503-505: the new prop
onImportDialogClose is used but not declared in PropTypes; add a PropTypes entry
for onImportDialogClose (e.g., PropTypes.func or PropTypes.func.isRequired
depending on whether it must always be provided) and include a short comment
describing the expected function signature (no args or a callback with an
event/object) to document usage.
1 participant
Fixes wso2/api-manager#4541
Issue URL: wso2/api-manager#4541
Problem
When importing an OpenAPI Definition for an existing API using the "Edit and Import" option in the Publisher Portal, the editor and import dialog remained open even after successfully saving the definition. This forced users to manually close both dialogs, creating a poor user experience.
Solution
Implemented a callback mechanism to properly manage dialog state between parent and child components. After a successful import save:
isImportingstate is properly resetChanges Made
Modified:
portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/APIDefinition/ImportDefinition.jsxonImportDialogCloseprop to receive close callback from parentuseEffecthookModified:
portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/APIDefinition/APIDefinition.jsxcloseImportDialogstate variable to store the close functionsetImportDialogClosemethod to receive the close function from child componentupdateSwaggerDefinitionmethod to close editor and import dialog after successful importonImportDialogClosecallback propBuild Information
portals/publisherportals/publisher/target/publisher.war(33 MB)Artifacts Replaced
publisherfolder inwso2am-4.6.0/repository/deployment/server/webapps/publisherfolderpublisher.warfrom build outputpublisherfolderTesting
No testing required for frontend changes (as per workflow guidelines).
Modified wso2am Pack Download
The complete modified
wso2am-4.6.0pack is available as a GitHub Actions artifact.🔗 Download from GitHub Actions
Artifact Details:
wso2am-4.6.0-issue-52.zipContents: Complete wso2am pack with the updated Publisher portal artifact ready to use.
Summary by CodeRabbit
New Features